Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve sc-service API #5364

Merged
merged 15 commits into from
Sep 2, 2024
Merged

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Aug 14, 2024

This improves sc-service API by not requiring the whole &Configuration, using specific configuration options instead. RpcConfiguration was also extracted from Configuration to group all RPC options together.

We don't use Substrate's CLI and would rather not use Configuration either, but some key public functions require it even though they ignored most of the fields anyway.

RpcConfiguration is very helpful not just for consolidation of the fields, but also to finally make RPC optional for our use case, while Substrate still runs RPC server on localhost even if listening address is explicitly set to None, which is annoying (and I suspect there is a reason for it, so didn't want to change the default just yet).

While this is a breaking change, most developers will not notice it if they use higher-level APIs.

Fixes #2897

@nazar-pc nazar-pc force-pushed the improve-sc-service-api branch 4 times, most recently from 7d0fef5 to a8b6767 Compare August 14, 2024 14:23
@nazar-pc
Copy link
Contributor Author

Would ExecutorConfiguration be a good name for use with new_wasm_executor that combines the following options?:

  • default_heap_pages
  • wasm_method
  • max_runtime_instances
  • runtime_cache_size

This is probably the last thing that I'd like to stop using &Configuration for now.

…ons, introduce `sc_service::config::RpcConfiguration` to replace a bunch of flat fields
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7094356

@dmitry-markin dmitry-markin added the T0-node This PR/Issue is related to the topic “node”. label Aug 23, 2024
…e-api

# Conflicts:
#	cumulus/polkadot-parachain/polkadot-parachain-lib/src/service.rs
#	cumulus/test/service/src/lib.rs
#	substrate/client/cli/src/config.rs
#	substrate/client/service/src/builder.rs
#	substrate/client/service/src/config.rs
#	substrate/client/service/src/lib.rs
@nazar-pc
Copy link
Contributor Author

Resolved merge conflicts after #4792 and would really appreciate if someone takes a closer look here

Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem justified to me. Thanks again for streamlined per-commit review process!

@dmitry-markin dmitry-markin requested a review from a team August 30, 2024 13:15
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nitpicks.

substrate/client/cli/src/lib.rs Outdated Show resolved Hide resolved
prdoc/pr_5364.prdoc Show resolved Hide resolved
@nazar-pc
Copy link
Contributor Author

nazar-pc commented Sep 2, 2024

Applied suggestions

@niklasad1
Copy link
Member

while Substrate still runs RPC server on localhost even if listening address is explicitly set to None, which is annoying (and I suspect there is a reason for it, so didn't want to change the default just yet).

I suppose you want to start your service without any rpc server then? Can you open an issue for it please?

I don't remember why we do it but that can changed if desired also with the latest refactoring it should be possible do addr: Some(vec![]) as workaround but I think it will throw an error when no rpc listen addr is found and bail early with the error.

@bkchr bkchr added this pull request to the merge queue Sep 2, 2024
Merged via the queue into paritytech:master with commit da65410 Sep 2, 2024
176 of 185 checks passed
@nazar-pc nazar-pc deleted the improve-sc-service-api branch September 3, 2024 06:54
x3c41a pushed a commit that referenced this pull request Sep 4, 2024
This improves `sc-service` API by not requiring the whole
`&Configuration`, using specific configuration options instead.
`RpcConfiguration` was also extracted from `Configuration` to group all
RPC options together.

We don't use Substrate's CLI and would rather not use `Configuration`
either, but some key public functions require it even though they
ignored most of the fields anyway.

`RpcConfiguration` is very helpful not just for consolidation of the
fields, but also to finally make RPC optional for our use case, while
Substrate still runs RPC server on localhost even if listening address
is explicitly set to `None`, which is annoying (and I suspect there is a
reason for it, so didn't want to change the default just yet).

While this is a breaking change, most developers will not notice it if
they use higher-level APIs.

Fixes #2897

---------

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor sc_service::config::Configuration struct
6 participants